Skip to content

Conversation

@VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Apr 30, 2025

This is a pure refactor. It removes a redundant internal interface from the PF bridge.

This started as a refactor but is actually a bug fix. Looks like we did not implement the PF portion of #2933 correctly. We also had a test which asserted on the wrong behaviour.

This PR fixes skipping PF WriteOnly attributes during tfgen. It also simplifies the PF code by removing a redundant interface.

AttrLike is a copy of the PF fwschema..Attribute which has WriteOnly implemented. This makes the method non-optional and adding it to an internal interface is not a breaking change.

@VenelinMartinov VenelinMartinov requested a review from a team April 30, 2025 10:19
@VenelinMartinov VenelinMartinov changed the title Remvoe redundant PF WriteOnly interface Remove redundant PF WriteOnly interface Apr 30, 2025
@VenelinMartinov VenelinMartinov force-pushed the vvm/remove_redundant_pf_writeonly_interface branch from d9ecf28 to bda2305 Compare April 30, 2025 10:22
@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Apr 30, 2025

@codecov
Copy link

codecov bot commented Apr 30, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 68.54%. Comparing base (4abc06e) to head (5d8a79c).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...kg/pf/internal/schemashim/object_pseudoresource.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3043      +/-   ##
==========================================
- Coverage   68.54%   68.54%   -0.01%     
==========================================
  Files         334      334              
  Lines       43376    43373       -3     
==========================================
- Hits        29734    29731       -3     
- Misses      11961    11963       +2     
+ Partials     1681     1679       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@VenelinMartinov VenelinMartinov changed the title Remove redundant PF WriteOnly interface Fix PF WriteOnly attributes skip Apr 30, 2025
}

func (s *attrSchema) WriteOnly() bool {
schema, ok := s.attr.(pfutils.AttrLikeWithWriteOnly)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the source of the bug - the type assert here is wrong as attr never implemented that interface.

@VenelinMartinov VenelinMartinov requested a review from t0yv0 April 30, 2025 10:48
@VenelinMartinov
Copy link
Contributor Author

❤️ tests:

error: [converter plugin terraform-1.0.20] downloading from : failed to download plugin: terraform-1.0.20: 403 HTTP error fetching plugin from https://get.pulumi.com/releases/plugins/pulumi-converter-terraform-v1.0.20-linux-amd64.tar.gz

Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there prod exposure e.g. in AWS or we caught this early?

Copy link
Contributor

@guineveresaenger guineveresaenger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks much better, thank you!

@VenelinMartinov
Copy link
Contributor Author

We caught this early - there are no PF write-only attributes in AWS.

@VenelinMartinov VenelinMartinov merged commit 9284425 into master Apr 30, 2025
75 checks passed
@VenelinMartinov VenelinMartinov deleted the vvm/remove_redundant_pf_writeonly_interface branch April 30, 2025 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants